fix: remove derived testIDs - #5099
Conversation
ef907a1 to
fde3c73
Compare
fde3c73 to
9daf5b0
Compare
satya164
left a comment
There was a problem hiding this comment.
few more test ID issues we should address:
DialogneedsoverlayTestIDChipneedscloseIconTestIDDrawer.ItemneedstestIDon theTouchableRippleCard.Titleneeds to forward thetestID
there are also some hardcoded test IDs left:
"activity-indicator"inSearchbar- remove it"options-select"and"select-page-dropdown-label"inDataTablePagination- remove unless necessary for some reason for testing, then add explicit props."list-image"- inListImage"tooltip-container"inTooltip"touchable-ripple-underlay"inTouchableRipple
I also notice hardcoded a11y labels in some cases. we should create follow ups to handle them. user needs to be able to override a11y labels for i18n.
|
|
||
| These components used to also derive test IDs for internal, implementation-only elements by appending a suffix to the `testID` prop (e.g. `${testID}-container`, `${testID}-icon`, `${testID}-outline`). Those internal test IDs have been removed entirely — they are no longer set even when you pass `testID` explicitly. Test IDs should only be used to interact with elements or assert content your users can reach, not to reach into a component's internal structure in tests; if you were relying on one of these internal IDs, query by role, label, text, or the component's own `testID`/`ref` instead. | ||
|
|
||
| The one exception is `BottomNavigation`, which still sets `${testID}-bar` on its internal `BottomNavigation.Bar` when you pass `testID`, since there's currently no other way to reference that node from outside the component. |
There was a problem hiding this comment.
remove it and add a barTestID prop to BottomNavigation that sets the test ID for the bar
There was a problem hiding this comment.
Done, updated in the new commit.
| You can specify a `testID` explicitly to restore both the component's own test ID and all of its derived test IDs above, using the same suffixes. | ||
| You can specify a `testID` explicitly to restore each component's own test ID. | ||
|
|
||
| These components used to also derive test IDs for internal, implementation-only elements by appending a suffix to the `testID` prop (e.g. `${testID}-container`, `${testID}-icon`, `${testID}-outline`). Those internal test IDs have been removed entirely — they are no longer set even when you pass `testID` explicitly. Test IDs should only be used to interact with elements or assert content your users can reach, not to reach into a component's internal structure in tests; if you were relying on one of these internal IDs, query by role, label, text, or the component's own `testID`/`ref` instead. |
There was a problem hiding this comment.
| These components used to also derive test IDs for internal, implementation-only elements by appending a suffix to the `testID` prop (e.g. `${testID}-container`, `${testID}-icon`, `${testID}-outline`). Those internal test IDs have been removed entirely — they are no longer set even when you pass `testID` explicitly. Test IDs should only be used to interact with elements or assert content your users can reach, not to reach into a component's internal structure in tests; if you were relying on one of these internal IDs, query by role, label, text, or the component's own `testID`/`ref` instead. | |
| These components used to also derive test IDs for internal, implementation-only elements by appending a suffix to the `testID` prop (e.g. `${testID}-container`, `${testID}-icon`, `${testID}-outline`). They have been removed entirely. | |
| If you were relying on internal test IDs, update your tests not to rely on internal implementation details and only interact with elements or assert content your users can reach, e.g.: query by role, label, text etc., or `testID` props accepted by the component. |
There was a problem hiding this comment.
Done, updated in the new commit.
| <Surface | ||
| backgroundColor={theme.colors.surfaceContainerHigh} | ||
| borderRadius={isBarMode ? theme.shapes.corner.extraLarge : cornerNone} | ||
| style={[styles.container, style]} | ||
| testID={testID ? `${testID}-container` : undefined} | ||
| elevation={elevation} | ||
| theme={theme} | ||
| > | ||
| <IconButton | ||
| role="button" | ||
| borderless | ||
| onPress={onIconPress} | ||
| iconColor={iconColor} | ||
| icon={ | ||
| icon || | ||
| (({ size, color }) => ( | ||
| <MaterialCommunityIcon | ||
| name="magnify" | ||
| color={color} | ||
| size={size} | ||
| direction={direction} | ||
| /> | ||
| )) | ||
| } | ||
| theme={theme} | ||
| aria-label={searchAccessibilityLabel} | ||
| testID={testID ? `${testID}-icon` : undefined} | ||
| /> | ||
| <TextInput | ||
| style={[ | ||
| styles.input, | ||
| { | ||
| color: textColor, | ||
| ...font, | ||
| ...Platform.select({ web: { outline: 'none' } }), | ||
| textAlign: inputTextAlign, | ||
| }, | ||
| isBarMode ? styles.barModeInput : styles.viewModeInput, | ||
| inputStyle, | ||
| ]} | ||
| placeholder={placeholder || ''} | ||
| placeholderTextColor={placeholderTextColor} | ||
| selectionColor={colors.primary} | ||
| underlineColorAndroid="transparent" | ||
| returnKeyType="search" | ||
| keyboardAppearance={dark ? 'dark' : 'light'} | ||
| role="searchbox" | ||
| ref={root} | ||
| value={value} | ||
| testID={testID} | ||
| {...rest} | ||
| /> | ||
| {loading ? ( | ||
| <ActivityIndicator | ||
| testID="activity-indicator" | ||
| style={styles.v3Loader} | ||
| /> | ||
| ) : ( | ||
| // Clear icon should be always rendered within Searchbar – it's transparent, | ||
| // without touch events, when there is no value. It's done to avoid issues | ||
| // with the abruptly stopping ripple effect and changing bar width on web, | ||
| // when clearing the value. | ||
| <View | ||
| pointerEvents={value ? 'auto' : 'none'} | ||
| testID={testID ? `${testID}-icon-wrapper` : undefined} | ||
| style={[ | ||
| !value && styles.v3ClearIcon, | ||
| right !== undefined && styles.v3ClearIconHidden, | ||
| ]} | ||
| > | ||
| <IconButton | ||
| borderless | ||
| aria-label={clearAccessibilityLabel} | ||
| iconColor={value ? iconColor : 'rgba(255, 255, 255, 0)'} | ||
| onPress={handleClearPress} | ||
| icon={ | ||
| clearIcon || | ||
| (({ size, color }) => ( | ||
| <MaterialCommunityIcon | ||
| name="close" | ||
| color={color} | ||
| size={size} | ||
| direction={direction} | ||
| /> | ||
| )) | ||
| } | ||
| testID={testID ? `${testID}-clear-icon` : undefined} | ||
| role="button" | ||
| theme={theme} | ||
| /> | ||
| </View> | ||
| )} | ||
| {shouldRenderTraileringIcon ? ( | ||
| <IconButton | ||
| role="button" | ||
| borderless | ||
| onPress={onTraileringIconPress} | ||
| iconColor={traileringIconColor || colors.onSurfaceVariant} | ||
| icon={traileringIcon} | ||
| aria-label={traileringIconAccessibilityLabel} | ||
| testID={testID ? `${testID}-trailering-icon` : undefined} | ||
| /> | ||
| ) : null} | ||
| {isBarMode && | ||
| right?.({ color: textColor, style: styles.rightStyle, testID })} | ||
| {!isBarMode && showDivider && ( | ||
| <Divider | ||
| bold | ||
| style={[ | ||
| styles.divider, | ||
| { | ||
| backgroundColor: colors.outline, | ||
| }, | ||
| ]} | ||
| testID={testID ? `${testID}-divider` : undefined} | ||
| /> | ||
| )} | ||
| </Surface> |
There was a problem hiding this comment.
since SearchBar has interactable elements in it, we should expose a way to access them with test IDs. add following props (similar to how we accept accessibility labels):
trailingTestIDclearTestIDsearchTestID
also I see it has a prop called traileringIcon which seems like a typo, idk a word named "trailering". lets also rename it to "trailing" in a follow-up PR (also check if the same typo is anywhere else), unless i'm missing something, then correct me.
There was a problem hiding this comment.
Done, updated in the new commit. I will create a follow-up PR and scan the repo for typos across the board.
| aria-label={iconAccessibilityLabel} | ||
| style={styles.icon} | ||
| testID={testID ? `${testID}-icon` : undefined} |
There was a problem hiding this comment.
Same as iconAccessibilityLabel, add iconTestID
There was a problem hiding this comment.
Done, updated in the new commit.
| onPress={dismissable ? onDismissCallback : undefined} | ||
| importantForAccessibility="no" | ||
| style={[styles.backdrop, backdropStyle, backdropTransitionStyle]} | ||
| testID={testID ? `${testID}-backdrop` : undefined} |
There was a problem hiding this comment.
add overlayTestID, similar to overlayAccessibilityLabel
There was a problem hiding this comment.
Done, updated in the new commit.
There was a problem hiding this comment.
the location of this testID seems wrong. it should be on the interactable element, i.e. the Pressable when present, otherwise on the Surface.
There was a problem hiding this comment.
Done, updated in the new commit. I also noticed now that the CardTitle siltently dropped the testID and never reached the rendered View so I fixed it by destructuring testID explicitly and forwarding it to the container View, consistent with how Card.Cover and Card.Actions already support it.
| aria-label={overlayAccessibilityLabel} | ||
| role="button" | ||
| onPress={onDismiss} | ||
| pointerEvents={visible ? 'auto' : 'none'} | ||
| style={styles.pressableOverlay} |
There was a problem hiding this comment.
Done, updated in the new commit.
- Drop `accessible` from the two List.Sections. With it, iOS collapsed each
section into a single accessibility element (XCUITest tree evidence in
evidence/a11y/). `testID` alone resolves `screenshot --crop-on` on both
platforms with identical rects, so the example app's accessibility tree
is unchanged by the hook.
- Rename the test ids to surface-example-{elevated,flat} so they cannot be
confused with the library defaults removed in #5088 / #5099. Baselines
renamed to __baselines__/<platform>/<story>.png on both platforms.
- Commit the evidence: raw diff JSON behind every table, break diff images,
issues.md, the a11y trees, and the final script runs.
- run.mjs: env.json mismatch and crop-size mismatch are hard failures
(exit 2 / 3) unless --force; --update can create a missing baseline;
always relaunch so the bundle is fresh (Fast Refresh was seen to stop
reaching Android); handle the Expo dev menu and dev launcher; because
the example app persists navigation state, go Back to the list root and
press the Surface row by position, not the header by label; --force-full
on every snapshot; summary.json written on every exit; --out.
- Re-measure the realistic break three times per platform at pre-declared
thresholds: bit-identical (iOS 10,179 px, Android 9,336 px at 0.02;
0 at 0.1) and identical to the original single captures.
- README: status, evidence links, a11y section, re-measurement section,
issues 13-18, updated Running-it.
Squash all appbar-improvements commits and replay them on top of latest main to avoid replaying 37 commits individually through main's unrelated history (notably the v2->v3 Appbar rewrite mid-branch, which produced large, non-semantic conflicts when rebased commit-by-commit). Also adapts the new Appbar/AppbarContent code to the testID conventions established by callstack#5099 (remove derived test IDs for internal, implementation-only elements): drops the `${testID}-title-text`/`${testID}-headline-text`/`${testID}-subtitle-text` derived test IDs on non-interactive text, updating the corresponding tests to query by text content instead, and updates the migration guide's "Test IDs" section accordingly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Motivation
Many components generated "derived" testIDs by appending a hardcoded suffix to the user-supplied
testIDprop (e.g.${testID}-container,${testID}-outline,${testID}-icon,${testID}-text) purely so tests could reach into internal, non-public parts of a component's render tree.This PR removes every derived testID suffix from the library and fixes up every test that depended on one, choosing between four outcomes on a case-by-case basis (see "Decisions by component" below):
ref) when the underlying behavior genuinely is public.toJSON()) snapshot when the check is a legitimate regression guard but has no public equivalent (an internal style value, an iconsource, an animated value) - this still catches regressions without the test reaching into a specific internal node by ID.One derived testID was deliberately kept (
BottomNavigation's${testID}-bar) because there is currently no public way to reach that node at all - see below.Affected components:
Surface,Button,IconButton,CrossFadeIcon,Appbar(AppbarHeader,AppbarContent),ProgressBar,Chip,ListItem,Card,FAB(Content,Shell,Menu,Extended),SegmentedButtonItem,CheckboxItem,DataTableCell,Searchbar,Snackbar,Banner,ToggleButton,Menu/MenuItem,Modal,Dialog,DrawerCollapsedItem,BottomNavigation/BottomNavigationBar.The
docs/6.x/docs/guides/migration.md"Test IDs" section is updated to reflect that these internal testIDs are now gone for good (previously it said they'd come back once you passedtestIDexplicitly, which was true right after #5088 but is no longer true after this PR), and calls out theBottomNavigationexception explicitly.Decisions by component
Grouped by what actually happened, since "removed a testID" covers several different situations:
Straightforward removal (no test depended on genuine public behavior)
Surface,Button,Chip,ListItem(-content),FAB.Content,FAB.Shell,FAB.Menu,ProgressBar,Snackbar,Dialog,MenuItem,BottomNavigationBar(-content,-content-wrapper) - the derived testID was dropped from the source and the corresponding assertions were rewritten to query by role/label/text where one existed, or the check was already covered elsewhere.IconButton: removed${testID}-iconon the inner icon. This one wasn't part of the original PR's scope and had zero references anywhere in the test suite - genuinely dead code, unrelated to any test.Testid moved to a more meaningful node instead of being deleted outright
CrossFadeIcon: previously had no testID of its own, only${testID}-previous/${testID}-currenton the two crossfadingAnimated.Views. Since the outer wrappingViewis the element that represents "this icon" from the outside, it now receives the plaintestIDdirectly, and the internal previous/current suffixes are gone.Menu:${testID}-surfacemoved to theSurfaceitself (testID={testID}), since theSurfaceis the menu's real visible content, not an implementation detail.${testID}-view(the animated positioning wrapper) was dropped since positioning is verified via snapshot instead (see below).Rewritten to assert public behavior
FABExtended.test.tsx-'expands to fit the measured label width'used to read the animated width offextended-fab-container. Rewritten to useFAB.Extended's ownrefprop (forwards throughShellto theSurface) withReanimated.getAnimatedStyle(ref.current)- no testID or DOM traversal, just a real forwarded ref any consumer could use.Modal.test.tsx- backdrop assertions rewritten togetByLabelText('Close modal'), a real interactive/accessible element. Surface opacity and inset-margin checks have no accessible-query equivalent, so those becametoJSON()snapshots.Searchbar.test.tsx- removed a duplicate clear-icon test, added a negative-case test (getByLabelText('clear', { includeHiddenElements: true })), dropped an internal-layout (marginLeft) assertion, and converted aborderRadiuscheck to a snapshot.Card.test.tsx,DrawerCollapsedItem.test.tsx,DataTable.test.tsx,CheckboxItem.test.tsx,SegmentedButton.test.tsx- style/prop/icon-source checks that read an internal testID (inDrawerCollapsedItem's case, via raw.propschains already flagged with an eslint-disable) converted totoJSON()snapshots or public queries (getByText, siblingtestID) where one existed. Same regression coverage, no internal-node access.Menu.test.tsx- position assertions rewritten to check the serialized tree plus a snapshot; shadow-style assertions now querytestIDdirectly sinceMenu's testID moved onto theSurface(see above).Kept as a deliberate, narrow exception
BottomNavigation.tsx:testID={testID ? \${testID}-bar` : undefined}on the nestedBottomNavigationBaris still there. Three tests (onIndexChange,onTabPress,onTabLongPress) need to fire a syntheticlayoutevent onBottomNavigationBar's root view before its tabs become pressable (they staypointerEvents: 'none'until the firstonLayoutfires, anduserEvent.pressgenuinely respectspointerEvents, unlikefireEvent).onLayoutdoesn't bubble in React Native, and neitherBottomNavigationnorBottomNavigationBarforwards aref, so there's currently no way to reach that node from outside without this testID. This isn't testing styles/props - it's the only way to get the component into an interactive state for a real interaction test. Flagging this explicitly for review: the clean long-term fix is adding realrefforwarding toBottomNavigationBar(mirroring whatFAB.Extended` already does), which is a real public-API addition and felt out of scope for a testID cleanup PR - happy to do it here instead if reviewers would rather not carry the exception.Related issue
None. This is an internal contributor task.
Test plan
yarn jest- full suite passes (55 suites, 674 passed + 1 skipped / 675 total, 233 snapshots).yarn eslint src- no violations (includingtesting-library/no-node-accessand the repo'sno-restricted-syntaxrule against raw.propsaccess).npx tsc --noEmit -p tsconfig.source.json(equivalent toyarn typecheck'stsc -bfor source files) - no type errors.testIDline (or replaces a targeted assertion with an equivalent full-tree one), with no unrelated structural or style changes.testIDattributes themselves.